fix: extract_json_from_stream corrupts output on balanced-but-non-JSON braces before the payload - #2456
Closed
ErenAta16 wants to merge 1 commit into
Conversation
… braces before the JSON payload
Bug
---
In MD_JSON mode, models often prefix the JSON payload with prose before
emitting the actual object, e.g.:
I'll pull out the fields now. {Note: keeping the original casing}
Here is the result: {"name":"Ada","age":30}
The non-streaming extractor (extract_json_from_codeblock, used by the
MD_JSON response parser for non-streaming responses) handles this
correctly: it scans for every bracket-balanced, JSON-parseable span in the
text and returns the last one, so it correctly returns
'{"name":"Ada","age":30}'.
extract_json_from_stream (used by the MD_JSON *streaming* response parser
in the openai/mistral/xai handlers) does not have this safety net. It
starts capturing as soon as it sees a '{' or '[' outside a string, and
once the bracket depth returns to zero it unconditionally yields whatever
it buffered, treats it as "the JSON", and keeps scanning for more objects.
It never checks that what it buffered actually parses as JSON. So the
brace-delimited aside "{Note: keeping the original casing}" gets emitted
as if it were a complete JSON object, and the real payload that follows
gets concatenated onto it:
{Note: keeping the original casing}{"name":"Ada","age":30}
That string is not valid JSON and downstream parsing of the streamed
response fails, even though the exact same text works fine through the
non-streaming code path.
Repro (before fix, run against instructor/v2/core/json.py directly):
>>> extract_json_from_codeblock(text)
'{"name":"Ada","age":30}' # correct
>>> "".join(extract_json_from_stream(chunks))
'{Note: keeping the original casing}{"name":"Ada","age":30}' # broken, fails json.loads
Root cause
----------
extract_json_from_stream buffers characters for a candidate JSON span and,
once the bracket stack empties (the span is "balanced"), immediately
yields the buffer without validating it is actually parseable JSON.
Bracket-balanced is a necessary but not sufficient condition for "this is
JSON" - plain prose can easily contain a balanced pair of braces that
isn't JSON at all.
Fix
---
When the bracket stack empties, try json.loads() on the buffered
candidate before yielding it. If it doesn't parse, discard the buffer and
keep scanning (mirrors the same-file extract_json_from_codeblock, which
already discards non-parseable bracket-balanced candidates the same way).
If it does parse, yield it exactly as before - behavior for the existing,
already-tested cases (fenced/plain JSON, multiple concatenated objects,
backtick handling, backslash handling) is unchanged. Applied the same fix
to the async counterpart, extract_json_from_stream_async, to keep the two
implementations in sync (verified via a fuzz comparison across both with
1/3/1000-char chunk sizes).
Testing
-------
- Added test_extract_json_from_stream_discards_non_json_brace_span_before_payload
and the async counterpart to tests/v2/test_json_helpers.py, following the
existing conventions in that file. Verified both fail on the pre-fix code
(via git stash) and pass on the fix.
- Ran the full tests/v2/test_json_helpers.py file (23 passed) and the
broader JSON extraction suites (tests/processing/test_json_extraction.py,
test_json_extraction_edge_cases.py, test_utils.py) - 102 passed.
- Ran the full tests/v2 directory and the broader tests/ tree (excluding
tests/docs and tests/llm, which need extra doc/LLM-specific deps).
Compared failures before and after the change via git stash: the same 10
failures exist on both (missing optional deps like google-genai/jsonref
and network-dependent auto_client/mistral tests) - none are caused by
this change.
Collaborator
|
Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In
MD_JSONmode, the model's response is often plain text with prosebefore the actual JSON payload, e.g.:
The non-streaming path (
extract_json_from_codeblock, used by theMD_JSONresponse parser for regular responses) handles this correctly -it scans for every bracket-balanced span that is actually parseable JSON
and returns the last one:
extract_json_from_stream(used by theMD_JSONstreaming parser inthe openai/mistral/xai handlers, see
instructor/v2/providers/openai/handlers.pyaround
extract_streaming_json) doesn't have the same safety net. Itstarts capturing as soon as it sees a
{/[outside a string, and oncethe bracket depth returns to zero it unconditionally yields whatever it
buffered - without checking it's actually JSON:
That's not valid JSON, so parsing the streamed response fails even though
the identical text works fine through the non-streaming code path.
Root cause
Bracket-balanced is necessary but not sufficient for "this is JSON" -
ordinary prose can easily contain a balanced brace pair that isn't JSON
at all (asides, notes, set notation, etc.).
extract_json_from_streambuffers a candidate span and, once the stack empties, yields it on faith.
Fix
When the bracket stack empties, validate the buffered candidate with
json.loads()before yielding it. If it doesn't parse, discard it andkeep scanning for the real payload - the same approach
extract_json_from_codeblockin the same file already uses for itscandidates. If it does parse, behavior is unchanged (fenced/plain JSON,
multiple concatenated objects, backtick/backslash handling all still
pass). Applied identically to the async counterpart,
extract_json_from_stream_async, to keep the two in sync.Testing
test_extract_json_from_stream_discards_non_json_brace_span_before_payloadand its async counterpart to
tests/v2/test_json_helpers.py. Verifiedboth fail on the pre-fix code (
git stash) and pass with the fix.tests/v2/test_json_helpers.py: 23 passed.tests/processing/test_json_extraction.py,tests/processing/test_json_extraction_edge_cases.py,tests/processing/test_utils.py,tests/test_utils.py: 102 passed.tests/v2andtests/(excludingtests/docs,tests/llmwhichneed extra optional deps): same pass/fail counts before and after the
change except for the 2 new tests. The 10 pre-existing failures
(missing
google-genai/jsonref, network-dependentauto_clienttests, one unrelated
mistraljson_schema_modetest) reproduceidentically on unmodified
mainviagit stash, confirming they'reunrelated to this change.